src/main/java/dev/noah/perplayerkit/gui/GUI.java (1)

32-40: Don't keep the legacy state helpers as silent no-ops.

If any legacy caller survives the migration, these methods now fail silently instead of surfacing the bad path. src/main/java/dev/noah/perplayerkit/listeners/KitMenuCloseListener.java still depends on this API, so I'd either remove the compatibility surface or deprecate it with an explicit failure/message while the old flow is being retired.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/dev/noah/perplayerkit/gui/GUI.java` around lines 32 - 40, These
legacy state helper methods (setInspectTarget, getAndRemoveInspectTarget,
removeKitDeletionFlag) currently no-op and silently hide callers like
KitMenuCloseListener; update them to explicitly fail fast by throwing an
UnsupportedOperationException (or at minimum log a clear warning and throw) with
a message directing callers to the new API/flow so surviving callers are
surfaced during testing, or alternatively implement the original behavior if
backward compatibility must be preserved; ensure the exception message names the
method (e.g., "GUI.setInspectTarget") and references KitMenuCloseListener
migration guidance.

src/main/java/dev/noah/perplayerkit/gui/configurable/ConfigurableGuiService.java (2)

912-922: Consider caching PlaceholderAPI availability check.

Bukkit.getPluginManager().getPlugin("PlaceholderAPI") is called on every text resolution. Since plugin presence doesn't change at runtime, caching this once during initialization would improve performance.

Proposed enhancement

Add a field and initialize it in the constructor:

private final boolean hasPlaceholderApi;

public ConfigurableGuiService(Plugin plugin) {
    this.plugin = plugin;
    this.guiConfigManager = new GuiConfigManager(plugin);
    this.hasPlaceholderApi = Bukkit.getPluginManager().getPlugin("PlaceholderAPI") != null;
    instance = this;
}

Then use it in resolveText:

     private String resolveText(String value, Player viewer, GuiContext context) {
         if (value == null) {
             return null;
         }

         String resolved = resolvePlainValue(value, viewer, context);
-        if (Bukkit.getPluginManager().getPlugin("PlaceholderAPI") != null) {
+        if (hasPlaceholderApi) {
             resolved = PlaceholderAPI.setPlaceholders(viewer, resolved);
         }
         return StyleManager.convertMiniMessage(resolved);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/dev/noah/perplayerkit/gui/configurable/ConfigurableGuiService.java`
around lines 912 - 922, The resolveText method repeatedly calls
Bukkit.getPluginManager().getPlugin("PlaceholderAPI") on every invocation; add a
private final boolean field hasPlaceholderApi to the ConfigurableGuiService
class, initialize it once in the constructor (ConfigurableGuiService(...)) by
checking Bukkit.getPluginManager().getPlugin("PlaceholderAPI") != null, and then
replace the runtime check in resolveText with a check of hasPlaceholderApi so
PlaceholderAPI.setPlaceholders(viewer, resolved) is only called when
hasPlaceholderApi is true.

---

924-935: Prefer StringBuilder over StringBuffer for single-threaded code.

StringBuffer is synchronized, adding unnecessary overhead. Since this method runs on the main thread per call, StringBuilder is more appropriate.

Proposed fix

     private String resolvePlainValue(String value, Player viewer, GuiContext context) {
         Matcher matcher = PLACEHOLDER_PATTERN.matcher(value);
-        StringBuffer buffer = new StringBuffer();
+        StringBuilder buffer = new StringBuilder();

         while (matcher.find()) {
             String replacement = getPlaceholderValue(matcher.group(1), viewer, context);
             matcher.appendReplacement(buffer, Matcher.quoteReplacement(replacement));
         }

         matcher.appendTail(buffer);
         return buffer.toString();
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/dev/noah/perplayerkit/gui/configurable/ConfigurableGuiService.java`
around lines 924 - 935, resolvePlainValue currently uses synchronized
StringBuffer via Matcher.appendReplacement/appendTail; replace it with an
unsynchronized StringBuilder to avoid unnecessary overhead: keep
PLACEHOLDER_PATTERN and the Matcher logic but change the loop to append the
literal text between matches and the replacement into a StringBuilder using
matcher.start()/matcher.end() (append the substring from the previous end index
to matcher.start(), then append the replacement from getPlaceholderValue), track
the last appended index, and after the loop append the remaining tail substring
before returning builder.toString(); update the method resolvePlainValue
accordingly.

src/main/java/dev/noah/perplayerkit/gui/configurable/GuiConfigManager.java (1)

21-26: Consider handling missing or malformed configuration more explicitly.

If guis.yml exists but contains invalid YAML, YamlConfiguration.loadConfiguration() returns an empty configuration without throwing. This could cause silent failures downstream when GUI sections are requested. Consider logging a warning when the file exists but produces an empty/invalid configuration.

Proposed enhancement

     public final void load() {
         if (!guiConfigFile.exists()) {
             plugin.saveResource("guis.yml", false);
         }
         guiConfiguration = YamlConfiguration.loadConfiguration(guiConfigFile);
+        if (guiConfiguration.getKeys(false).isEmpty()) {
+            plugin.getLogger().warning("guis.yml appears to be empty or invalid");
+        }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/dev/noah/perplayerkit/gui/configurable/GuiConfigManager.java`
around lines 21 - 26, The load() method currently calls
YamlConfiguration.loadConfiguration(guiConfigFile) but doesn’t detect when the
file exists yet yields an empty/invalid configuration; update load() to check
the loaded guiConfiguration for emptiness (e.g., no top-level keys/sections)
after YamlConfiguration.loadConfiguration(guiConfigFile) and if empty: log a
warning via the plugin logger including the file name (guiConfigFile) and that
the YAML appears invalid, then either attempt to restore the default with
plugin.saveResource("guis.yml", true) or copy the bundled default into place and
re-load; reference the existing symbols load(), guiConfigFile, guiConfiguration,
plugin.saveResource and YamlConfiguration.loadConfiguration when implementing
this check and recovery.

src/test/java/dev/noah/perplayerkit/gui/configurable/BundledGuisConfigTest.java (1)

27-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Close the resource stream after loading the YAML.

The InputStream from getResourceAsStream is wrapped in an InputStreamReader but never closed. While the JVM will clean up at test shutdown, using try-with-resources is best practice and prevents resource leak warnings in CI.

♻️ Proposed refactor

     `@BeforeAll`
     static void load() {
-        var stream = BundledGuisConfigTest.class.getClassLoader().getResourceAsStream("guis.yml");
-        assertNotNull(stream, "guis.yml missing from resources");
-        config = YamlConfiguration.loadConfiguration(new InputStreamReader(stream, StandardCharsets.UTF_8));
+        try (var stream = BundledGuisConfigTest.class.getClassLoader().getResourceAsStream("guis.yml")) {
+            assertNotNull(stream, "guis.yml missing from resources");
+            config = YamlConfiguration.loadConfiguration(new InputStreamReader(stream, StandardCharsets.UTF_8));
+        }
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/dev/noah/perplayerkit/gui/configurable/BundledGuisConfigTest.java`
around lines 27 - 32, Update the `@BeforeAll` load() method to wrap the resource
InputStreamReader in try-with-resources, ensuring the stream from
getResourceAsStream is closed after YamlConfiguration.loadConfiguration
completes while preserving the existing missing-resource assertion.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes